fix: VRAM and memory leaks in window, session, output, and multi-GPU paths - #2500
fix: VRAM and memory leaks in window, session, output, and multi-GPU paths#2500minyek wants to merge 11 commits into
Conversation
|
Resolved the conflict with master (upstream's new KeyboardLayoutState::refresh landed at the same spot in refresh() as the KMS cache cleanup — kept both); the approval was auto-dismissed by the push, mind re-approving? |
Drakulix
left a comment
There was a problem hiding this comment.
Looks mostly good. Just one nitpick.
| } | ||
| self.last_renderer_cleanup = Instant::now(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of adding a new periodic cleanup function, should we be able to simply do a cleanup (or schedule a cleanup with loop_handle.insert_idle after output (re-)configuration?
There was a problem hiding this comment.
I don't think that'd fix the issue as the queue is populated at drop time, not import time — surfaces imported during (re-)configuration only become garbage later, when the window closes or the client exits. A cleanup right after configuration would run before that garbage exists and then never again, so e.g. a client exiting an hour after the last hotplug would stay pinned in VRAM until the next mode change. Since the resources are queued at arbitrary points (window close, client exit, cache eviction inside smithay), there's no single event that covers them all — hence the throttled drain (a make_current + empty try_iter at most every 2s).
We could make this event-based with a smithay change (e.g. a notifier/ping when a cleanup queue gets work), but for this PR I decided to keep the change contained to cosmic-comp.
There was a problem hiding this comment.
Right. I think ideally we would simply have a method on the GraphicsApi/MultiRenderer and GlesRenderer to invalidate all caches. We know we don't need them and cleaning them up periodically still seems quite unnecessary to me.
Would you consider making a smithay PR to add such methods?
There was a problem hiding this comment.
Yes, I'm happy to make the smithay change and I agree it's a better solution. I'm away on holiday, so when I'm back next week I'll complete my testing and submit the smithay PR. Once that's approved, I'll update this PR.
A client that disconnected while minimized left its MinimizedWindow holding strong references indefinitely, trapping the window's GPU textures. Add an IsAlive impl for MinimizedWindow and retain only live entries in Workspace::refresh and WorkspaceSet::refresh. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
The cursor image_cache accumulated animation frames without bound as it grew across shape changes. Track the last cursor icon and clear the cache when the shape changes. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
…aces pending_activations entries for Wayland surfaces that died before completing were never removed and accumulated indefinitely. Drop them during Shell::refresh once the surface is no longer alive. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
Removing an output left its LockSurface in SessionLock.surfaces, retaining the surface's GPU buffer. Remove the output's entry when the output is torn down. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
The retain filter for ActivationKey::X11 always returned true, so entries for X11 windows that died before completing mapping accumulated indefinitely. Keep an X11 entry only while a matching pending window still exists. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
postprocess_textures kept the node's GlesTexture render buffers after a DRM node was removed, leaking them on GPU hot-unplug. Drop the node's entry in node_removed. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
OutputZoomState, stored in Output.user_data(), holds an IcedElement whose outputs set contains the same Output, so the Output never drops after disconnect — leaking it, its user_data, and the GPU textures held by the IcedElement. Call output_leave() on the zoom element before removing the output to break the cycle. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
A multi-GPU client can be active on several render nodes. The disconnect handler broke out of the device loop after the first node it freed, leaking the client's id (and its imported buffers) on every other device it had used. Remove the client from every device, then refresh used devices once if any were freed. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
Moves onto the revision carrying `Renderer::invalidate_caches` and the multi-GPU cache methods the next commit calls into. The bump crosses three API changes: - The tablet protocol handling moved from `wayland::tablet_manager` to `input::tablet` and became event-struct based, with an explicit `TabletSeatHandler::ToolFocus` and per-frame `frame()` calls. Axis state is now sent as one `AxisFrame` built from the libinput event's changed-axis flags, which is what the previous sequence of individual setter calls expressed. - `PointerConstraintRef::deactivate` takes the state, surface and pointer, because smithay now calls `PointerConstraintsHandler::remove_constraint` from it; the handler also receives the constraint being removed. - `remove_constraint` therefore runs with the per-surface constraint mutex held and, on the pointer-leave path, from inside pointer dispatch. Applying the cursor position hint there deadlocks the compositor, since `apply_cursor_hint` retakes that mutex and drives `pointer.motion`, so the seat lookup and the warp are deferred to an idle callback. The hint is now honoured when the constraint is deactivated as well as when it is destroyed, which is what `zwp_locked_pointer.set_cursor_position_hint` specifies; clearing the hint on the first makes the second a no-op. The lockfile also moves `drm-ffi` to 0.9.1 and `drm-sys` to 0.8.1, which the new revision requires, and drops `rand` with it. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
The renderers owned by the main thread's `GpuManager` - used for output (re-)configuration, screenshots and image-copy captures - draw only on demand, and a renderer only flushes its GL destruction queue and only re-imports its caches when it draws. A dead client's textures therefore stay queued, and a live client's imports stay cached, until whatever happens to draw next, which may be minutes or hours away. Both pin the buffers in VRAM for that whole time. The per-output render threads have no such problem: they draw every frame. Tie the two halves of the cleanup to the events that create the garbage rather than to a timer: - Destruction schedules a drain. `buffer_destroyed` and `CompositorHandler::destroyed` set a flag that `refresh` acts on with `GpuManager::cleanup_texture_cache`, so the queued deletions are flushed on the next event-loop pass instead of at the next draw. Batching through the flag keeps a client exit that destroys hundreds of surfaces to one drain. - Infrequent draws invalidate afterwards. Output (re-)configuration, screenshots and image-copy captures call `invalidate_caches` once they are done, since the imports they just cached provide no benefit before the next such draw and the next one re-imports what it needs. Both go through the `GpuManager` cache methods rather than a hand-rolled loop over `devices_mut`. Such a loop cannot reach the buffers cached for copying between a render and a target node: those belong to the manager, keyed by node pair, rather than to any one renderer. `offscreen_renderer` builds exactly such a render-to-target renderer for screencopy and screenshots, so a per-device loop left a full output-sized dmabuf per node pair pinned in VRAM. Captures drain rather than invalidate, because a screencast session captures repeatedly and its imports are worth keeping between frames. Screenshots on the Glow path do neither: the winit and X11 backends redraw every frame, and the KMS software fallback holds no VRAM. The per-output render threads get the same treatment for consistency, which supersedes `f86cd933`: their end-of-draw loop over `devices_mut` predates the manager-level methods and is what those methods were added to replace. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
Removing a connector dropped its `Surface` without joining the thread it owns. A bare drop only signals the thread to end. If a later reconfigure takes the DRM compositor write lock before the detached thread finishes its in-flight frame, the thread blocks on the read lock, never observes `End`, and so never releases its renderer, swapchain and postprocess offscreens. That strands a full set of output-sized render targets in VRAM for the rest of the session: compositor VRAM does not return to its previous floor after a display is unplugged, and each subsequent unplug adds another set. Join the thread so those resources are released before the connector's removal completes. Unlike `apply_config_for_outputs`, this path holds no compositor lock, so the join cannot deadlock against the thread's own read lock; the two other `drop_and_join` call sites rely on the same property. Developed with AI assistance (Claude Code); all changes have been reviewed and are understood by the author.
0f558cc to
39aead4
Compare
|
Smithay/smithay#2139 has landed, so this branch is rebased onto current 1. 2. 3. Validation: a multi-day soak plus a scripted regression pass on an instrumented build with a GPU-resource census (27 censuses over 2.5 h, dual-output NVIDIA), covering output power-cycles, client churn, screencopy, workspace overview, and popups/zoom/minimize/cross-output moves. Every compositor-side counter returns to baseline or saturates flat, with no panic, GL error or DRM commit failure; the only residual growth is the known NVIDIA driver pooling, tracked separately with a standalone reproducer. One caveat: the cursor-hint deferral postdates the soak and is covered by manual testing of pointer-locking clients only. |
We already have an update in progress that also implements tablet grabs and other features coming with the smithay update here: #2725 I'll ping you once that is merged and the smithay version used by cosmic-comp is new enough. I'd prefer to do that myself and then have you simply drop this commit and rebase the fixes on top of it. Thanks for validating all the fixes though! |
Summary
Nine independent fixes to compositor-owned leaks. Each keeps GPU buffers or
compositor objects alive after the window, session, or output that needed them is
gone, so memory climbs during minimize, zoom, monitor-hotplug, multi-GPU, and
activation workflows and never returns to the idle baseline.
This is the cosmic-comp half of a two-part investigation into a steady VRAM climb
on the proprietary NVIDIA driver; the companion
smithay PR fixes library-level leaks in the
allocator and GLES renderer. These fixes are self-contained against current
masterand need no smithay change. A residual VRAM climb on NVIDIA — memory notreclaimed for sampled, cross-process dmabuf imports — reproduces with no
compositor code and is an NVIDIA driver issue, not a bug in either PR.
The NVIDIA driver issue has been reported here
What's fixed
MinimizedWindowholding strong references, trapping GPU textures; now droppedvia an
IsAliveretain inWorkspace::refresh/WorkspaceSet::refresh.image_cacheaccumulated animation frames withoutbound across cursor-shape changes; now cleared when the shape changes.
pending_activationsentries for Waylandsurfaces that died before completing were never removed; now pruned in
Shell::refresh.ActivationKey::X11retain filter alwaysreturned
true, so entries for X11 windows that died before mappingaccumulated; now kept only while a matching pending window exists.
LockSurfaceinSessionLock.surfaces; now removed on output teardown.postprocess_textureskept a DRM node'sGlesTextures after the node was removed; now freed on GPU hot-unplug.Output → OutputZoomState → IcedElement → Outputkept the output alive after disconnect; broken by calling
output_leave()onthe zoom element before removing the output.
output (re)configuration, so they never drained their GL destruction queues,
pinning imported client buffers in VRAM indefinitely; now drained periodically
from
refresh().device loop after the first node, leaking the client's id (and its imported
buffers) on every other device it used; now removed from all devices.
Testing
Both binaries were instrumented for this hunt — this compositor and its smithay
dependency — and validated with a SIGUSR1 resource census over a multi-day
dual-output NVIDIA session. Every targeted container stayed zero or bounded under
load: minimized windows, zoom states, pending activations, and session-lock
surfaces returned to zero, and the main-thread GL cleanup queues drained fully
(15.4M framebuffers cycled through and freed over three days). The only residual
is an NVIDIA driver-side VRAM pool that reproduces with no compositor code, not a
compositor leak. Compiles against the pinned smithay (rev
85f83ab).AI assistance
This work was developed with AI assistance (Claude Code); use of AI-generated
code is disclosed in the commit messages per the contribution guidelines. All
changes have been reviewed and are understood by the author.
Checklist